Skip to content

Add scalar_fieldmatrix #2289

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

imreddyTeja
Copy link
Member

@imreddyTeja imreddyTeja commented Apr 14, 2025

closes #2306
This PR adds scalar_fieldmatrix, a function which takes in a field matrix, and returns a field matrix with keys that are associated with matrix fields of either uniform scaling or columnwisebandmatrices of scalars.

This also makes the get_index for field matrices return a field instead of a broadcasted object when the keys used to index the field matrix will result in a columnwisebandmatrices of scalars.

get_internal_entry, which is called by get_index for FieldMatrices, is not complete, but works for everything that scalar_fieldmatrix needs. Any indexing that was previously supported is still supported. An example of something that could be added, is if an entry is a tuple of axistensors, indexing into only the axis tensor components.

  • Code follows the style guidelines OR N/A.
  • Unit tests are included OR N/A.
  • Code is exercised in an integration test OR N/A.
  • Documentation has been added/updated OR N/A.

@imreddyTeja imreddyTeja force-pushed the tr/fieldmatrix-functions-for-jacobian branch 2 times, most recently from 67fd30e to 65b036a Compare April 15, 2025 17:16
@imreddyTeja imreddyTeja force-pushed the tr/fieldmatrix-functions-for-jacobian branch from 65b036a to 3f81735 Compare April 15, 2025 17:36
@imreddyTeja imreddyTeja marked this pull request as ready for review April 15, 2025 21:40
@imreddyTeja imreddyTeja force-pushed the tr/fieldmatrix-functions-for-jacobian branch from 2405aca to 02bb025 Compare April 24, 2025 18:34
@imreddyTeja imreddyTeja force-pushed the tr/fieldmatrix-functions-for-jacobian branch from d91cf59 to e1c1c22 Compare May 8, 2025 23:42
eltype(x.parent)
inner_type_ignore_adjoint(x::DataLayouts.AbstractData) = eltype(x)
inner_type_ignore_adjoint(x::Adjoint) = typeof(x.parent)
inner_type_ignore_adjoint(x) = typeof(x)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also support when x is a broadcasted object that contains an Adjoint?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It currently is never used where x could be broadcasted, but maybe it should be added so this could be used elsewhere? Although this should be used with caution, because it is a bit of a hack (it is only used with axis vectors)

@imreddyTeja imreddyTeja force-pushed the tr/fieldmatrix-functions-for-jacobian branch 3 times, most recently from 03c0a76 to ef4ead4 Compare May 23, 2025 16:51
@imreddyTeja imreddyTeja force-pushed the tr/fieldmatrix-functions-for-jacobian branch 2 times, most recently from 2c6c054 to 52cf757 Compare May 29, 2025 18:21
Add a function to convert a FieldMatrix where each matrix entry
has an eltype of some struct into a FieldMatrix where each entry
has an eltype of a scalar.

Add additional tests for scalar_matrixfields

Use @test_all in tests

Make suggested changes to tests and field_name_dict.jl

Revert unrolled_findfirst

Clean up field matrix tests and add support for DiagonalMatrixRows

CamelCase struct name

Clean up tests and get_scalar_keys

wip backup

Minimal working with allocs

WIP1

WIP more allocs fix

Assorted cleanup

Fix dx/dx case

reduce code duplication; fix example

Add gpu test

further cleanup, extend diagonalrow

fix names test and comments

Add  docs

docs bugfix

remvoe bad refs

fix docs formatting
@imreddyTeja imreddyTeja force-pushed the tr/fieldmatrix-functions-for-jacobian branch from 52cf757 to 13633ca Compare May 29, 2025 22:08
Copy link
Member

@dennisYatunin dennisYatunin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Teja! We should eventually try to simplify the scalar indexing logic, as it's ended up very entangled with the AxisTensor internals, but I think you've captured every case we could possibly support. The new section in the docs is also helpful. This will be really useful for implementing the sparse autodiff Jacobian in ClimaAtmos.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add scalar_fieldmatrix(::fieldmatrix)
3 participants